Skip to content

Replace scrl with a native scroll implementation #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
hirasso merged 6 commits into from
Jun 6, 2025
Merged

Replace scrl with a native scroll implementation #88

hirasso merged 6 commits into from
Jun 6, 2025

Conversation

@hirasso
Copy link
Member

@hirasso hirasso commented May 31, 2025 •
edited
Loading

Closes #87

Description

  • remove the dependency to gmrchk/scrl, decreasing the bundle size while increasing the feature set
  • support scrolling the closest scrollingElement as seen from an anchor element (overflowing div, dialog,...)
  • keep the hooks working
  • keep the autoKill feature working (cancel the scroll upon user interaction)
  • keep the maximum scroll position detection (prevent a sudden stop at end of scroll container)
  • still worth a major version I think, as the default scroll animation changes significantly with this vs. scrl
  • maybe the major version would be a good chance to add support for scrolling on the x-axis if necessary, as well?

I'm surprised how well this whole thing works! The "scrollend" event I'm using to dispatch the "scroll:end" hook doesn't work in safari just yet – but I've never actually used it, so I'd be fine with it being missing.

Checks

  • The PR is submitted to the main branch
  • The code was linted before pushing (npm run lint)
  • All tests are passing (npm run test) OMG this plugin doesn't have any tests yet 😂
  • Tested in local playground with both window and overlays
  • The documentation was updated as required

@hirasso
Replace scrl with a native scroll implementation
8b472a1 
- support scrolling the closest scrolling element from an anchor element
- keep the hooks
- keep the `autoKill` feature (cancel scroll upon user interaction)
@hirasso hirasso requested a review from daun May 31, 2025 16:30
@hirasso hirasso mentioned this pull request May 31, 2025
Closed
4 tasks
@daun
Copy link
Member

daun commented May 31, 2025

Fantastic! 🥳 I'll give this a try soon. Major version sounds good, since the scroll duration is now different and some sites might rely on that. Also, all in favor of finally adding tests! Although I suspect there's a reason it's currently untested — lots and lots of flaky Playwright magic...

@hirasso
Copy link
Member Author

hirasso commented May 31, 2025 •
edited
Loading

Yeah... let alone the fixtures we'll need for writing proper tests 🤯😅

@daun
Copy link
Member

daun commented Jun 6, 2025 •
edited
Loading

I love it, works great! Scrolling is quite a bit smoother when using the native implementation.

Would be amazing to extend this and both scroll the overflow container into view, as well as scroll the container to reveal the nested anchor element. Recursive scrollIntoView, so to say. I'll see if I can make it work without too much contortion...

And I'd like to think of a nicer API than overwriting the scrollTo method. This screams "replaceable hook" to me :)

@hirasso
Copy link
Member Author

hirasso commented Jun 6, 2025

Should we still do these things in separate PRs? This one changes a lot already.

@hirasso
Copy link
Member Author

hirasso commented Jun 6, 2025

Maybe we could merge these things into a next branch. And publish that branch as a beta to npm?

@daun
Copy link
Member

daun commented Jun 6, 2025

Yes to both! I've started tinkering with this on a separate branch. Let's merge this one into develop or next and take it from there. Personally, I'm fine with testing this locally using symlinks or yalc, I'm a bit hesitant to juggle the whole npm release tag thing 😅

@daun
Copy link
Member

daun commented Jun 6, 2025

Maybe we can revert the changes to the readme where the linter added ; after every line of JSON :(

daun
daun previously approved these changes Jun 6, 2025
View reviewed changes
@daun daun mentioned this pull request Jun 6, 2025
Merged
5 tasks
@daun
Copy link
Member

daun commented Jun 6, 2025

I've created PR #89 we can use as a launchpad for further improvements once this is one merged 🥬

@hirasso hirasso changed the base branch from master to next June 6, 2025 20:20
@hirasso hirasso merged commit 7a8ea8e into next Jun 6, 2025
@hirasso hirasso deleted the feat/native-scrolling branch June 6, 2025 20:21
@hirasso
Copy link
Member Author

hirasso commented Jun 6, 2025

Merged this into next. We can base all upcoming PRs to that branch.

@hirasso hirasso mentioned this pull request Jun 6, 2025
Merged
13 tasks
@hirasso
Copy link
Member Author

hirasso commented Jun 6, 2025

While at it I also renamed the master branch to main. #90 is already based on that branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daun daun daun left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add anchor scroll support inside overflowing elements

3 participants

@hirasso @daun